ci: answer "is a boringssl bump safe" by test, not by argument by aaylward · Pull Request #178 · muchq/smithy-cpp

aaylward · 2026-08-04T13:17:25Z

What

Assessing a boringssl snapshot bump (e.g. Renovate's #177) today still rests partly on argument — reading upstream commit logs, reasoning about MVS. This PR converts each of those remaining arguments into a red-or-green check:

TLS ceiling pins (runtime/tests/http/beast_client_test.cc): the suite pinned the 1.2 floor, AEAD ciphers, and ALPN, but not the ceiling actually reached. Two new tests assert an uncapped handshake lands on TLS 1.3 (server-side raw probe) and that the client reaches a TLS-1.3-only listener (CappedTlsServer grows an optional floor). A boringssl bump that quietly moved the default protocol version now fails a test instead of passing every floor check. Session resumption is deliberately not pinned: neither transport configures it, so there is no behavior to pin.
Resolution invariant (examples/bazel-consumer/boringssl-resolution-check.sh, wired into the consumer job): the consumer's resolved boringssl must equal the MODULE.bazel pin. Bazel's MVS picks the highest version any module requests, so a transitive dep could drag the TLS library past the pinned-and-tested version with every leg still green; that skew is now a failure.
Lockfile freshness leg (lockfiles job): bazelisk mod deps --lockfile_mode=error for the root and consumer modules, so a MODULE.bazel edit lands together with its lockfile and dependency PRs stay auditable from the diff — same posture as the codegen job's golden-freshness check. The leg earned its keep immediately: the checked-in consumer lockfile was already stale (rules_shell 0.8.0 was added without a repin); regenerated here.
Canary workflow (.github/workflows/canary.yml): twice a week (and on demand), force the newest BCR boringssl onto both module graphs via single_version_override and run the TLS-pinning runtime suites plus the out-of-tree consumer's acceptance tests (which terminate real TLS handshakes). It answers "is the next snapshot safe?" before Renovate opens the PR, and stands in for the consumer whose other deps force a newer boringssl than the pin. Its test steps pass --lockfile_mode=off explicitly: the override makes the checked-in lockfiles stale by construction, and relying on Bazel's current default of update would break if the default ever flips to error.
Renovate config: osvVulnerabilityAlerts for the merge-urgency direction ("is it safe not to merge?"), and a repin note on every bazel-module PR mirroring the existing maven_install.json note.

make lockfiles joins verify (the Makefile mirrors ci.yml one target per job), the consumer target runs the resolution check, and docs/development.md lists the new target.

Testing

make lockfiles passes locally on both modules after the consumer repin, and error mode was verified to bite: bumping the boringssl pin without a repin fails with Missing checksum … Please run bazel mod deps --lockfile_mode=update.
./boringssl-resolution-check.sh passes (resolved boringssl@0.20260730.0 matches smithy_cpp's pin).
The modified test TU compiles clean under -Wall -Wextra against boringssl 0.20260730.0, Boost 1.90, and googletest 1.17 headers; clang-format --dry-run --Werror is clean. The sandbox's egress policy blocks a toolchain archive the full bazel test needs, so the two new TLS tests run for the first time in this PR's CI matrix — 7 jobs exercise them: the four-toolchain matrix, both ASan/UBSan legs, and the coverage run. (The TSan step covers only the concurrency suites, which don't include beast_client_test.)
Both workflow files YAML-parse; renovate.json is valid JSON.

Checklist

Tests added/updated for the change
bazel test //... and (cd codegen && gradle build spotlessCheck) pass locally (sandbox egress blocks one toolchain fetch; validated by this PR's CI instead)
Formatting clean (clang-format, buildifier, spotless)
Architectural decisions recorded as an ADR (if applicable — none: CI/test infrastructure only)

🤖 Generated with Claude Code

https://claude.ai/code/session_015oE5zkytVkgzmKNPSP2CWM

Generated by Claude Code

Merging a boringssl snapshot bump (e.g. Renovate's #177) today rests partly on reading upstream commit logs. Convert each remaining argument into a red-or-green check: - Pin the TLS ceiling, not just the floor: an uncapped handshake must land on TLS 1.3 (server-side probe), and the client must reach a 1.3-only listener (CappedTlsServer grows an optional floor). A boringssl default drifting would now fail beast_client_test instead of passing every floor test. - Resolution invariant in the consumer job: the consumer's resolved boringssl must equal the MODULE.bazel pin, so MVS silently selecting a higher version a transitive dep requested becomes a failure (boringssl-resolution-check.sh). - Lockfile freshness leg: bazelisk mod deps --lockfile_mode=error for the root and consumer modules, so a MODULE.bazel edit lands with its lockfile and dependency PRs stay auditable from the diff. The leg earned its keep immediately: the consumer lockfile was already stale (rules_shell 0.8.0 was added without a repin) — regenerated here. - Canary workflow: twice a week, force the newest BCR boringssl onto both module graphs via single_version_override and run the TLS- pinning runtime suites plus the consumer acceptance tests — failing *before* the next Renovate bump opens, and standing in for the consumer whose other deps drag boringssl past our pin. - Renovate: osvVulnerabilityAlerts for the advisory direction ("is it safe NOT to merge"), and a repin note on every bazel-module PR mirroring the maven_install.json posture. make lockfiles joins verify; the consumer target runs the resolution check; docs list the new target. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oE5zkytVkgzmKNPSP2CWM

cursor · 2026-08-04T14:31:43Z

Review

Verdict: approve with a few small follow-ups. The PR does what it claims — turns “is this boringssl bump safe?” into red/green checks — and CI is fully green (including the new lockfiles leg and consumer resolution check). The design matches the existing consumer/golden-freshness posture.

What works well

TLS ceiling pins are the right complement to the existing floor tests: uncapped server negotiation must land on TLS 1.3, and the client must reach a 1.3-only listener. Session resumption correctly left unpinned.
Resolution invariant closes a real MVS hole; wiring it like model-evolution-check.sh is consistent.
Lockfile freshness already paid for itself (stale consumer lock after rules_shell 0.8.0).
Canary (force newest BCR via single_version_override, run TLS suites + consumer) is the right “front-run Renovate” shape; version string sanitization before splicing into MODULE.bazel is good.
Renovate osvVulnerabilityAlerts + bazel-module repin note mirror the existing maven note.

Findings

Canary should set --lockfile_mode explicitly (low/medium)
Appending single_version_override makes the checked-in lockfile stale for that run. Today Bazel’s default is update, so CI’s ephemeral workspace is fine — but a future default flip to error would make the canary fail before any test runs. Prefer --lockfile_mode=update or off on both canary test steps (same guidance as overrides in docs/quickstart.md).
docs/development.md prose is half-updated (nit)
The piecemeal target list includes lockfiles, but the intro still says verify covers “bazel tests, gradle build + format, golden freshness, lint” — no lockfile freshness — and the command block never shows make lockfiles / bazelisk mod deps --lockfile_mode=error.
PR description overclaims TSan coverage (nit)
New tests live in beast_client_test; the TSan leg only runs session_registry_test, async_event_stream_test, beast_transport_test, and beast_websocket_test. ASan/UBSan cover them via //...; TSan does not. Harmless, but the “7 jobs including … TSan” line is wrong.

Optional / non-blocking

Canary failure visibility: scheduled workflows fail quietly unless someone watches Actions. A failure issue / notification would make “red before Renovate” more reliable.
boringssl-resolution-check.sh sed assumes a single-line bazel_dep(name = "boringssl", version = "…"). True today; a multi-line dep would silently break the check.
BCR yanked versions: sort -V | tail -1 can pick a yanked snapshot. Unlikely, but filtering yank metadata would harden the canary.

Not concerns

Consumer lockfile growth (Go SDK facts, rules_android extensions) looks like normal re-resolve churn from the overdue repin, not a new runtime dep.
Skipping session-resumption pins is correctly justified.
Root MODULE.bazel.lock left alone is fine — the new leg passed, so it was already fresh.

Bottom line: merge-ready after considering the canary --lockfile_mode hardening and the small docs/description cleanups; nothing that should block if you’re comfortable taking those as follow-ups. Current BCR latest (0.20260803.0) matches Renovate #177, so once this lands the canary is the natural pre-flight for that bump.

Review follow-ups (PR #178): the canary's single_version_override makes the checked-in lockfiles stale by construction, so its test steps now pass --lockfile_mode=off explicitly instead of relying on Bazel's current default of update — a future default of error would have failed the run before any test executed. docs/development.md's verify summary and underlying-commands block now mention lockfile freshness alongside the other gates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oE5zkytVkgzmKNPSP2CWM

aaylward · 2026-08-04T14:34:28Z

Addressed the three findings in 63b1b14:

Canary lockfile mode — both canary test steps now pass --lockfile_mode=off explicitly (matching the repo's existing override guidance), with a comment explaining that the appended single_version_override makes the checked-in lockfiles stale by construction. off over update because the lockfile has nothing to contribute to a deliberately-overridden throwaway run.
docs/development.md — the verify summary now lists lockfile freshness, and the underlying-commands block shows bazel mod deps --lockfile_mode=error.
TSan overclaim — PR description corrected: the 7 jobs are the four-toolchain matrix, both ASan/UBSan legs, and coverage; the TSan step covers only the concurrency suites, which don't include beast_client_test.

The optional items (canary failure issue/notification, multi-line bazel_dep robustness in the resolution check, yank filtering in the BCR query) are left as follow-ups per the review.

Generated by Claude Code

aaylward merged commit f12c660 into main Aug 4, 2026
16 checks passed

aaylward deleted the claude/smithy-cpp-pr-177-safety-l3qrdm branch August 4, 2026 23:17

Navigation Menu

Uh oh!

ci: answer "is a boringssl bump safe" by test, not by argument - #178

ci: answer "is a boringssl bump safe" by test, not by argument#178
aaylward merged 2 commits into
mainfrom
claude/smithy-cpp-pr-177-safety-l3qrdm

aaylward commented Aug 4, 2026 •

edited

Loading

Uh oh!

cursor Bot commented Aug 4, 2026

Uh oh!

aaylward commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

aaylward commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Testing

Checklist

Uh oh!

cursor Bot commented Aug 4, 2026

Review

What works well

Findings

Optional / non-blocking

Not concerns

Uh oh!

aaylward commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aaylward commented Aug 4, 2026 •

edited

Loading